Skip to content

refactor(harness): share project roots [Agent Map 01/15] - #819

Merged
ynadge merged 3 commits into
mainfrom
review/agent-map-01-project-roots
Sep 6, 2026
Merged

refactor(harness): share project roots [Agent Map 01/15]#819
ynadge merged 3 commits into
mainfrom
review/agent-map-01-project-roots

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Maintenance or refactor

Problem and motivation

Studio used different path and project-root rules on the server and in the browser. A descendant session or a different spelling of a root could acquire a different project identity or sidebar group.

Summary and scope

Extract shared path comparison and deterministic project-root selection, then use it in the Studio catalog, server root discovery, and browser project tree. Existing durable project roots remain authoritative across descendants, multiple roots, and restart.

Canonicalize Windows and symlink aliases before catalog deduplication, retaining the existing durable project ID and metadata. Graph lookup and descendant-session resolution use the same root identity.

How this increment fits

Shared path identity and catalog migration are usable on their own. Session capability changes begin in part 2.

Stack and review boundary

  • Part 01 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: main.
  • Current head: a2c8d7f6f3612e36373c5e0cc486a2601ba9d3aa; 1,941 changed lines across 12 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #804. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3148. This packaging follows the maintainer-approved 15-PR split.

Validation

Fresh local checks against a2c8d7f6f3612e36373c5e0cc486a2601ba9d3aa:

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: Windows drive and UNC aliases, real and symlink roots, recent-directory eviction, descendant sessions, graph lookup, and durable identity after restart.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Existing catalog records migrate without changing valid project identities. No public session contract changes.
  • Changeset: Included: .changeset/durable-project-root-compatibility.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #819 (round 1)

1. Studio-derived workspace scopes are published with keys the graph resolver cannot resolve

packages/harness/src/server/index.ts:2495-2512

listWorkspaceScopesAndRetain now returns studioProjectCatalog.reconcile(studioWorkspaceScopeCatalog.list()) merged with the legacy list. Two things downstream still key off the legacy catalog only:

  • createSystemGraphRouter({ scopeResolver: workspaceScopeCatalog }) (index.ts:3116) resolves workspaceKey from recentDirs ∪ session cwds. Any root the new derivation contributes that is not in that set has no resolvable key → 404 "Workspace not found" (server/system-graph.ts:48).
  • retained (index.ts:2497) is built from scopes, the legacy list, before the merge. Studio-only roots are therefore never leased by systemGraphWatcher/systemGraphStore, and activeSystemGraphScopes deletes them on every refresh (index.ts:2500-2503).

The new roots are not an edge case — they are the headline of this PR:

  • a live session at /proj/sub whose durable binding is /proj, where /proj is not in recentDirs (the closure maps it to the root at index.ts:1252-1266);
  • retainedProjectSessionRoots, which exists specifically to re-add roots evicted from recentDirs (index.ts:1296-1310);
  • promoted parents from holdingProjectFor, which are by construction neither a recentDir nor a session cwd.

Before this change the input to reconcile was the same list retain/resolve used, so every published key resolved. Fix is one source of truth: either point scopeResolver/retained at studioWorkspaceScopeCatalog, or compute retained from the merged result.

2. Tightened root uniqueness bricks an existing Windows catalog — no migration

packages/harness/src/core/studio-project-catalog.ts:204, 252-266

Root-binding uniqueness moved from byte-exact localRootRef to pathComparisonKey, which lowercases Windows drive/UNC paths. canonicalGraphPath is path.resolve-based and does not case-fold, so a catalog written by 0.14.x on Windows can legitimately hold C:\Work\Proj and c:\work\proj as two bindings. Under this diff those collapse to one key, parseCatalog throws malformed_state, and load() (:357-388) has no quarantine or repair path — it rethrows on every call, forever. The whole Agent Map catalog goes permanently unavailable and the user silently falls back to the legacy rail (index.ts:2513 catch). Tightening a persisted invariant needs a load-time dedupe/merge, not a hard reject.

3. No changeset for behavior changes in a published package

@sapiom/harness is on npm; CONTRIBUTING.md:195 requires a changeset when a change "affects a published package's behavior". This one does, regardless of stack staging — path comparison becomes case-insensitive for Windows drive/UNC paths for every consumer of these helpers, and the workspace-scope list the SPA renders is composed differently. The PR base is main, so merging ships those changes with no CHANGELOG line. A patch changeset naming the Windows case-folding is the minimum; the "N/A because the stack is unmerged" rationale does not hold while the base is main.

4. The new server wiring has no test

The 121 new lines in server/index.ts — the projectSessionRoot mapping, the pinnedRoots handoff, the retainedProjectSessionRoots fallback, the merge in listWorkspaceScopesAndRetain — carry no new test. src/server/studio-workspace-wiring.test.ts, the only server suite the PR body cites, is unchanged and predates this PR (added in #766). The 231-line project-roots.test.ts covers the pure module well; the finding in §1 lives entirely in the untested half. A wiring test that starts the server with a session cwd below a durable root not present in recentDirs, then fetches the published workspaceKey, would catch it.

5. Internal prose now ships in the npm tarball

packages/harness/src/shared/project-roots.ts:242-246, 254-256

These comment blocks moved from web/src/lib/project-tree.ts — where Vite strips them — into src/, which tsc emits with comments preserved (removeComments is unset in tsconfig.json, and files ships dist). So dist/shared/project-roots.js now publishes, verbatim and unretractably, a description of a captured internal dogfood install (org-dogfood.json in the design prototype: 75 agents, 8 recentDirs, 41 distinct session cwds) and "Three agents were on screen twice this way on one real machine." No company is named, so this is not a leak of the first order, but it is internal operating detail addressed to an anonymous npm consumer. Reword to the generic shape ("a large real install: ~75 agents across 41 session cwds") and drop the fixture filename. Bare SAP-#### refs are an existing repo norm (57 in src/) and are not in scope here.


Verdict: Request changes — §1 is a live correctness regression in the path this PR exists to unify, and §2 has no recovery once it fires.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #819 (round 2, follow-up)

Delta reviewed: 4cbb298b..34177a98 (1 commit, 6 files).

New finding

1. The scope merge is keyed by resolve(), not the comparison key this PR introduces — one root can still publish twice

packages/harness/src/server/index.ts:2500-2502

byRoot keys both sides on resolve(scope.cwd), a raw lexical resolve. Every other join in this PR keys on pathComparisonKey(canonicalGraphPath(...)), and LocalWorkspaceScopeCatalog derives workspaceKey from canonicalGraphPath — which calls realpathSync.native, so it resolves symlinks and, on Windows/macOS, on-disk casing.

So when the durable binding's localRootRef (stored canonical) and the recentDirs spelling differ only by a symlink hop or by case — /tmp/x vs /private/tmp/x, c:\work\proj vs C:\Work\Proj — the two entries hash to different byRoot keys and both survive the merge, while workspaceKeyForRoot gives them the same workspaceKey. state.workspaceScopes then carries two rows for one project, one with projectId and one without, and the SPA renders two sidebar groups: the exact duplicate-identity failure the PR description says it removes. Key the map with pathComparisonKey(canonicalGraphPath(scope.cwd)).

Earlier findings

All five are addressed:

  • §1 — retained is now computed after the merge and scopeResolver resolves from the same published list. Resolved.
  • §2 — parseCatalog reverts the byte-exact invariant and collapses case aliases in-place, persisted under the write lock; cross-project ambiguity now falls through to unassignedScopes instead of throwing malformed_state. Resolved; the trade-off (an ambiguous root stays without a projectId until a user merges the projects) is documented in the code.
  • §3 — .changeset/durable-project-root-compatibility.md, patch, names the Windows case-folding. Resolved.
  • §4 — studio-workspace-wiring.test.ts now covers the descendant-session-after-eviction path end to end, including the retain sets and a 200 from the graph route. Resolved.
  • §5 — the dogfood install figures and fixture filename are gone from shared/project-roots.ts. Resolved.

Nothing the earlier round got wrong.

Verdict: Request changes — one line, finding 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant